fix: add plan-time validation to key_prefix in autolink reference resource (#3176)#3277
fix: add plan-time validation to key_prefix in autolink reference resource (#3176)#3277LeC-D wants to merge 3 commits into
Conversation
|
👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with |
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
austenstone
left a comment
austenstone
left a comment
There was a problem hiding this comment.
Left a concrete regression-test suggestion so the fix is covered at plan time as well as in the schema.
There was a problem hiding this comment.
This validation looks right. I think the missing piece is a regression test that proves invalid prefixes now fail during terraform plan, since that’s the behavior this PR is fixing.
Because this PR only changes this file, I can’t attach a native GitHub suggestion directly to github/resource_github_repository_autolink_reference_test.go, but something along these lines would lock it in:
t.Run("rejects invalid key_prefix at plan time", func(t *testing.T) {
randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum)
repoName := fmt.Sprintf("%srepo-autolink-%s", testResourcePrefix, randomID)
config := fmt.Sprintf(`
resource "github_repository" "test" {
name = "%s"
description = "Test autolink validation"
}
resource "github_repository_autolink_reference" "invalid" {
repository = github_repository.test.name
key_prefix = "PTFY25"
target_url_template = "https://example.com/PTFY-<num>"
}
`, repoName)
resource.Test(t, resource.TestCase{
PreCheck: func() { skipUnauthenticated(t) },
ProviderFactories: providerFactories,
Steps: []resource.TestStep{
{
Config: config,
ExpectError: regexp.MustCompile(`must only contain letters, numbers, or .* must not end with a number`),
},
},
})
})That would make sure we don’t regress back to the current plan/apply mismatch later.
There was a problem hiding this comment.
Good catch! Added a resource.UnitTest sub-test that passes key_prefix = "PTFY25" (ends with a digit) and asserts a plan-time error matching must not end with a number. The test uses providerFactories and doesn't require a real GitHub account.
|
Good call! Added a |
LeC-D
force-pushed
the
fix/autolink-key-prefix-validation
branch
from
d721ddc to
49a967c
Compare
|
Rebased on upstream |
| ValidateDiagFunc: validation.ToDiagFunc(validation.StringMatch( | ||
| regexp.MustCompile(`^[a-zA-Z0-9.=+:/#_-]*[a-zA-Z.=+:/#_-]$`), | ||
| "must only contain letters, numbers, or .-_+=:/# and must not end with a number", | ||
| )), |
There was a problem hiding this comment.
suggestion: use 2 validations, one for suffix and one for allowed chars.
And the must not end with could be simplified to \D$ I think
There was a problem hiding this comment.
Done — split into two StringMatch validators inside validation.All(): one for allowed characters (^[a-zA-Z0-9.=+:/#_-]+$) and one for the digit-suffix check using \D$ as you suggested. Error messages are now separate and targeted.
|
Applied @deiga's review feedback: split the |
…egrations#3176) GitHub's API enforces that key_prefix: - must only contain letters, numbers, or .-_+=:/# - must not end with a number Without client-side validation, terraform plan succeeds with an invalid key_prefix (e.g. 'PTFY25') but terraform apply fails with a 422 from the GitHub API, giving a confusing plan/apply inconsistency. Added ValidateDiagFunc using a regexp that mirrors the API constraints, so invalid key_prefix values are caught at plan time.
Adds a plan-time validation test asserting that a key_prefix ending in a digit (e.g. PTFY25) is rejected before any API call is made. Addresses review comment from @austenstone.
Per review by @deiga: use two StringMatch validators inside validation.All() — one for allowed characters (^[a-zA-Z0-9.=+:/#_-]+$) and one for the digit-suffix check (\D$) — instead of a single combined regex. This makes the error messages more precise.
LeC-D
force-pushed
the
fix/autolink-key-prefix-validation
branch
from
3e5df52 to
b1015f6
Compare
|
Rebased on upstream |
|
Rebased on upstream |
|
Rebased on upstream |
|
Rebased on upstream |
|
@LeC-D Sorry that this has taken long to get to. Would you be available to rebase this (possibly multiple times) during the next week? We're trying to get 6.13.0 out and will try to merge as many ~ready PRs as is reasonable |
stevehipwell
left a comment
stevehipwell
left a comment
There was a problem hiding this comment.
@LeC-D please could you rebase this PR?
| ValidateDiagFunc: validation.ToDiagFunc(validation.All( | ||
| validation.StringMatch( | ||
| regexp.MustCompile(`^[a-zA-Z0-9.=+:/#_-]+$`), | ||
| "must only contain letters, numbers, or the characters .-_+=:/#", | ||
| ), | ||
| validation.StringMatch( | ||
| regexp.MustCompile(`\D$`), | ||
| "must not end with a number", | ||
| ), | ||
| )), |
There was a problem hiding this comment.
| ValidateDiagFunc: validation.ToDiagFunc(validation.All( | |
| validation.StringMatch( | |
| regexp.MustCompile(`^[a-zA-Z0-9.=+:/#_-]+$`), | |
| "must only contain letters, numbers, or the characters .-_+=:/#", | |
| ), | |
| validation.StringMatch( | |
| regexp.MustCompile(`\D$`), | |
| "must not end with a number", | |
| ), | |
| )), | |
| ValidateDiagFunc: validation.ToDiagFunc(validation.StringMatch( | |
| regexp.MustCompile(`^[a-zA-Z0-9.=+:/#_-]+\D$`), | |
| "must only contain letters, numbers, or the characters .-_+=:/# and not end with a number", | |
| )), |
There is no reason to use two validators for this. Also could you confirm the logic for the start of the prefix can actually include all or the allowed characters?
4 participants
Summary
Fixes #3176
Problem
terraform planaccepts any value forkey_prefixingithub_repository_autolink_reference, butterraform applycan fail with a 422 from the GitHub API if the value is invalid:This creates a confusing plan/apply inconsistency where the plan shows a clean diff but apply fails.
Fix
Added
ValidateDiagFuncto thekey_prefixschema field using a regexp that mirrors GitHub's API constraints:.-_+=:/#Both
regexpandvalidationwere already imported in this file (used bytarget_url_template). No new dependencies needed.Testing
Existing acceptance tests use
key_prefixvalues likeTEST1-,TEST2-, etc. which all pass the new validation. Invalid values likePTFY25(ends with digit) will now be caught at plan time.